Skip to content

Conversation

@adityagarg06
Copy link
Contributor

@adityagarg06 adityagarg06 commented Aug 22, 2023

Hey @lindapaiste, I've been working on adding styled component to the Sidebar. I've come across a situation where the class .sidebar_project-options needs to extend from .dropdown-open-right. However, since I can't directly use extends in styled components, I'm considering moving the dropdown-open-right code to the mixins.scss file as mixins. Would it be a good idea, or do you have any other suggestions?

It's coming up like this without the dropdown-open-right
1

.sidebar__project-options {
  @extend %dropdown-open-right;
  display: none;
  width: 100%;
  max-width: #{180 / $base-font-size}rem;
  .sidebar--project-options & {
    display: flex;
  }
}

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@lindapaiste
Copy link
Collaborator

Hey @lindapaiste, I've been working on adding styled component to the Sidebar. I've come across a situation where the class .sidebar_project-options needs to extend from .dropdown-open-right. However, since I can't directly use extends in styled components, I'm considering moving the dropdown-open-right code to the mixins.scss file as mixins. Would it be a good idea, or do you have any other suggestions?

This has been something that has been on my mind and I'm not sure of the best long-term solution. The best temporary solution to keep us moving for now is to keep a className on the sidebar div in order to apply the CSS mixin. In the SCSS, you would delete all of the styles that you migrated to styled-components, but keep the @extend mixin.

.sidebar__project-options {
  @extend %dropdown-open-right;
}
<SidebarProjectOptions className="sidebar__project-options">

@lindapaiste
Copy link
Collaborator

I have some thoughts about conditional rendering and I don't know if others agree @raclim @dewanshDT. This is just my opinion but I don't like hiding content via CSS display: none. This code (and other components with similar setups) could definitely be simplified a lot if we were to handle the conditional rendering in the JS instead. And I think it's better for accessibility because we don't need to worry about adding aria-hidden attributes if the hidden elements aren't in the DOM.

@adityagarg06 The way you have handled the showing and hiding is consistent with how it's being done currently in the .scss so that's good ✔️. It's probably something that we should discuss and get everyone on board with before making the changes that I am suggesting.


Instead of this:

const SidebarProjectOptions = styled.ul`
  display: none;
  width: 100%;
  max-width: ${remSize(180)};
  ${(props) =>
    props.open &&
    css`
      display: flex;
    `}
`;
<SidebarProjectOptions open={projectOptionsVisible}>
  /*...*/
</SidebarProjectOptions>

It could be:

const SidebarProjectOptions = styled.ul`
  display: flex;
  width: 100%;
  max-width: ${remSize(180)};
`;
{projectOptionsVisible && (
  <SidebarProjectOptions>
    /*...*/
  </SidebarProjectOptions>
)}

@dewanshDT
Copy link
Collaborator

I totally agree with this

@adityagarg06
Copy link
Contributor Author

@lindapaiste I have made all the changes that you required, Sorry for the late response.

@raclim raclim added Type:Task Tasks tied specifically to developer operations and maintenance Area:CSS For styling or layout issues handled with CSS/SASS labels Jan 26, 2024
@vaishnavi192
Copy link

This issue is closed right? @adityagarg06 @lindapaiste

@raclim raclim added the Closing Soon Will be closed in 7 days due to inactivity or resoltuion label May 30, 2024
@raclim
Copy link
Collaborator

raclim commented May 31, 2024

Thanks for your work on this! Due to the amount of time that's passed since this pull request was opened, I'm going to close this for now. I'm sorry that we ended up not being able to merge this in, but please feel free to reopen a new pull request for this issue!

@raclim raclim closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area:CSS For styling or layout issues handled with CSS/SASS Closing Soon Will be closed in 7 days due to inactivity or resoltuion Type:Task Tasks tied specifically to developer operations and maintenance

Projects

Status: Styled-Components

Development

Successfully merging this pull request may close these issues.

5 participants